fix(service-automation): move driver text out of engine.ts's last three store-seam log messages, with a per-seam #4632 level verdict (#6299) - #6498
Merged
Conversation
…ast three store-seam log messages (#6299) All three catches sit around the `SuspendedRunStore` driver and rendered their failure by interpolating the thrown value's `.message` into the log MESSAGE. `ObjectLogger.write()` adds one `<ts> <LEVEL>` head per call, so a multi-line driver error turned ONE record into several physical lines of which only the first was greppable. The cause now goes to the logger's structured slot via `describeThrownForLog`, closing the #5048 / #5575 / #5636 / #5661 / #5737 / #5912 / #6230 family for this file. The #4632 level was judged per seam rather than batch-copied from #6230: - forgetSuspendedRun -> raised to `error` (durability): the hot cache is dropped before the store delete, so a failed delete leaves the suspension consumed in-process and the durable row alive, to be re-listed and re-resumed after the next restart. - cancelRun -> raised to `error` (durability): an unreadable store makes the read report "no such suspended run", so the method returns `false` (its contract's idempotent success) and the cancellation is silently skipped. - listSuspendedRunsDurable -> stays `warn` (functional): nothing claimed-persisted failed to land; the listing degrades to the in-memory cache and the message now says so out loud. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USNUyHEr7uaU6MoEWXitei
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 1 package(s): 5 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
os-project-manager
marked this pull request as ready for review
August 8, 2026 04:06
This was referenced Aug 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6299
The last three seams of
engine.tsthat interpolated a foreign cause into a logmessage. All three are catches around the
SuspendedRunStoredriver:forgetSuspendedRun(store.delete),cancelRun(store.load) andlistSuspendedRunsDurable(store.list). Eighth instalment of the family#5048 / #5575 / #5636 / #5661 / #5737 / #5912 / #6230.
The shared half: the message
ObjectLogger.write()adds exactly one "timestamp + level" record head percall, so a driver error carrying newlines turns one record into several
physical lines of which only the first is greppable. The cause now goes to the
logger's structured slot via the package's existing
describeThrownForLog—zero new vocabulary. Slot position follows the
Loggercontract:error(message, error?, meta?)takes it third (theErrorslot stays emptyper #5575, so no stack is shipped on every record),
warn(message, meta?)takesit second.
Measured on the restored concatenation, in
pretty: the three-line driver errorproduced 3 physical lines and
serve's boot-quiet filter retained 1 —and that one carried no driver fact. That is PR #6297's measurement reproduced
at these seams.
The half that is NOT shared: three separate #4632 verdicts
#6230's "the level stays warn" was deliberately not copied. Each seam was
judged on its own consequence path, and they do not agree.
1.
forgetSuspendedRun— raisedwarntoerror(durability)this.suspendedRuns.delete(run.runId)runs on the line above the try, andthis method is the single choke point every consumption of a suspension passes
through (resume / terminal failure / cancel). So a failed
store.delete()leaves the suspension gone in-process while the durable row survives. Every
caller still reports success —
resume()returns a successful result,cancelRun()returnstrue,recordLogwrites a terminal record — and thesurviving row is read straight back out after the next restart:
rearmSuspendedWaitTimerslists it,resume()rehydrates it throughloadSuspendedRunStrict's store read, and a continuation that has already runruns again. Nothing retries the delete.
This is the shape the durability gate's own vocabulary already grades
errorfor the metadata store —
deleteMetaItemFromLoader(#5259): "the surviving rowis read straight back out of storage … so the 'deleted' item reappears and
survives every restart."
2.
cancelRun— raisedwarntoerror(durability)The failed read is turned into "no such suspended run" and the method returns
false, which its own contract documents as idempotent success (alreadyterminal / unknown) — so the cancellation is skipped while the call reads
clean.
The only in-repo caller measures the cost.
plugin-approvals' revise-windowrecall (
packages/plugins/plugin-approvals/src/approval-service.ts) never readsthe boolean at all — it only catches a throw, and grades that throw
errorwith "cancelRun after revise-window recall failed — the run may be stranded"
(#4420). A store-read failure produces precisely that stranded run without
firing that alarm: the request is marked
recalled, the record lock isreleased,
resumeErrorstays undefined, and the run stays parked in the storeto be re-armed and resumed by the next restart, inside a flow whose approval has
already been withdrawn.
Why #6230's verdict does not transfer:
loadSuspendedRunis a declaredbest-effort reader for incidental callers, and
resumeInternaltakes the strictform exactly where the difference matters.
cancelRunhas no strict alternativeand its degradation decides a write.
3.
listSuspendedRunsDurable— stayswarn(functional)Nothing the system claims to have persisted failed to land: the rows are intact,
still resumable by id, and the next boot's
rearmSuspendedWaitTimersstillre-arms them off its own
store.list()— whichbuiltin/wait-node.tsgrades
errorprecisely because that failure breaks the promise to resumethem. This one breaks no promise, so #4632's judgment question answers no.
What is wrong here is the shape #5186 owns: an answer invented for a read
that failed, i.e. a silently short list. That rule's remedy is propagation
(rethrow, or a discriminated result), which changes this method's return
contract and is outside this issue — so the record now states the shortfall out
loud instead. Reachability is also the weakest of the three: the method has no
production consumer in-repo (only
engine.test.ts) and is not on theAutomationServicespec contract, so nothing decides anything on the listtoday. Raising it would alarm for the duration of an outage on a read nobody
acts on — #4632's mirror-image misuse, the trap #6230 avoided.
Why the levels need test pins
pnpm check:durability-log-levelgrades none of the three: its write-rulevocabulary (
DURABILITY_CRITICAL_CALLEES) does not nameSuspendedRunStore'smethods, and its read-seam rule's scan roots (
packages/metadata,metadata-protocol,objectql) do not reach this package. The level pins inthe new test are the only thing holding these verdicts.
Stream side effect, stated as a side effect
ObjectLoggerrouteswarnto stdout anderrorto stderr, andserve'sboot-quiet window wraps
process.stdout.writealone — so raising two levelsalso moves those two records off the boot buffer's drop surface. That is a
consequence of the routing, not a reason either level was raised, and it is
measured in a dedicated case so a future "tidy these back to one level" cannot
silently undo it.
Tests
New
suspended-run-store-consume-log-cause.test.ts— 20 cases, real bytes off areal
ObjectLogger(a spy proves what the seam called, not what thedownstream splitter sees). Per seam: message stays single-line under a
multi-line driver error, in
jsonand inpretty; the cause lands in thecorrect argument slot; a level pin matching that seam's verdict including
which stream it lands on; behaviour unchanged; and a single-line driver renders
identically so the fix is unconditional.
Reverse verification, direction predicted before running: restoring the
concatenation turns 16 of 20 red. The 4 that stay green are the three
behaviour pins (unchanged by this PR, correctly) and — reported rather than
papered over —
listSuspendedRunsDurable's level pin, which cannot go redhere because this PR does not change that seam's level. That pin guards a future
raise, not this PR's own regression; before-green/after-red was never available
for it and is not claimed.
Sweep: the issue's "last batch" premise is false
The issue closes with "治完这三处,
engine.ts这一族就全净了". It is not. The threein scope are fixed; a content sweep of the same file found 13 more sites of
the same shape, including
persistSuspendedRun25 lines above site 1, on thesame
SuspendedRunStoredriver seam (store.save(), alreadyerror, messagestill concatenated). Per Prime Directive #10 none is touched here — filed
separately as a finding.
Verification
pnpm --filter @objectstack/service-automation test— 69 files, 826 tests, all passtsc --noEmitand ESLint clean on both changed files.github/workflows/lint.yml, run one by one: all pass exceptcheck:i18n,check:i18n-coverageandcheck:type-check-debt, which fail identically on a pristineorigin/mainin this worktree (they need a full workspace build this worktree does not have) — environmental, not from this changecheck:nul-bytespasses; a beyond-the-gate self-scan caught a raw0x1Bthat the copiedbootFilterRetainscomment had grown, before the file was tracked and therefore before the gate could see it (the.claude/skills/**的 markdown 不被任何门禁扫描 —— check:nul-bytes 只看 JS/TS,check:doc-authoring 的 ROOTS 不含 .claude/ #4890 shape). Removed, and the comment now names the incident.Generated by Claude Code